Skip to content

Conversation

@danbev
Copy link
Member

@danbev danbev commented Feb 4, 2025

This commit updates the comments in the code to reflect that the function common_lcs() is actually computing the length of the longest common substring between two sequences (consecutive elements), not the longest common subsequence.

The motivation for this change it to clarify the intent of the function.

This commit updates the comments in the code to reflect that the
function common_lcs() is actually computing the length of the longest
common substring between two sequences (consecutive elements), not the
longest common subsequence.

The motivation for this change it to clarify the intent of the function.
@ngxson
Copy link
Collaborator

ngxson commented Feb 4, 2025

Tbh, I'm not sure if some of your recent changes are even necessary. Totally appreciate contribution here and there, but what I absolutely don't want is to be too nitpicking. IMO the priority should spending time focus on fixing bugs and implementing new functions.

If you want to continue doing small changes like this, please expect that I'll allocate less time to review them.

Regarding this PR, I believe this is based on the famous Longest Common Subsequence leetcode problem, so I think it's totally ok to keep it that way in this context.

@danbev
Copy link
Member Author

danbev commented Feb 4, 2025

The reason for opening this is that I was surprised when reading through server code and the comment in server.cpp and in common_lcs says it is finding the subsequence (of the cached tokens and the tokens in the prompt). But from what I can see this is not the case and it is returning the longest substring.

If you want to continue doing small changes like this, please expect that I'll allocate less time to review them.

Note taken, and I'll be more careful about opening PRs moving forward, my intention was not to be a burden 👍

Regarding this PR, I believe this is based on the famous Longest Common Subsequence leetcode problem, so I think it's totally ok to keep it that way in this context.

There is also Longest Common Substing and perhaps this is where the confusion lies.

@danbev danbev closed this Apr 21, 2025
@danbev danbev deleted the longest-common-substring branch August 13, 2025 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants